Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom-pages-sizes to CLI flags #8917

Merged

Conversation

alexcrichton
Copy link
Member

Enables -W custom-page-sizes from the CLI to run this proposal.

@alexcrichton alexcrichton requested a review from a team as a code owner July 8, 2024 19:15
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team July 8, 2024 19:15
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should maybe eventually have a for_each_config_option macro to define both the CLI flags and the Config methods or something...

@fitzgen fitzgen added this pull request to the merge queue Jul 8, 2024
@alexcrichton
Copy link
Member Author

Agreed yeah, that'd kind fall under #8784 I think

@fitzgen
Copy link
Member

fitzgen commented Jul 8, 2024

Ah yes, I forgot about that issue. 100%.

Merged via the queue into bytecodealliance:main with commit a184619 Jul 8, 2024
37 checks passed
@alexcrichton alexcrichton deleted the custom-page-sizes-cli-flag branch July 8, 2024 21:36
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 9, 2024
fitzgen added a commit that referenced this pull request Jul 9, 2024
* Add custom-pages-sizes to CLI flags (#8917)

* Update dependency on `wit-bindgen` (#8911)

* Update dependency on `wit-bindgen`

Updating to the latest released version.

* Add vets

* Fix build of test-programs

* Fix panic with invalid DWARF file indices (#8913)

I'm not entirely sure what causes this but Wasmtime shouldn't panic with
invalid DWARF. In general (#5537) Wasmtime's support for DWARF needs to
be rewritten, but in the meantime let's play whack-a-mole with panics
and try to paper over issues.

Closes #8884
Closes #8904

* Wasmtime: Pop GC LIFO roots even when there is no GC heap (#8899)

* Wasmtime: Pop GC LIFO roots even when there is no GC heap

We can create and root `i31ref`s without ever allocating a GC heap for the
store, so we can't guard popping LIFO roots on the presence of a GC heap or else
we risk unbounded growth of the LIFO root set.

* Fix build with the gc feature disabled

---------

Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
@Mossaka
Copy link
Member

Mossaka commented Aug 18, 2024

We should maybe eventually have a for_each_config_option macro to define both the CLI flags and the Config methods or something...

I was looking at how the CLI flags and Config are defined and it seems like the grouping rules are very different between the two. For CLI, flags are grouped under struct names such as "Optimize", "Codegen", "Debug", etc, while in the Config, fields are grouped to different struct names, such as "Tunables". The lack of the 1-1 mapping makes it hard to define CLI flags from the Config sturct.

Perhaps, adding a macro like [cli(Optimize)] or others to Config fields helps to generate these fields in their corresponding CLI structs?

In addition, the methods in the Config are not a 1-1 map to the fields as well... It might be a headache to write the macro that generates the methods according fields.

@fitzgen
Copy link
Member

fitzgen commented Aug 19, 2024

I think even something that defines the public API of Config and keeps that in sync with the CLI flags, but doesn't define the implementation of those Config methods, would be helpful. And that should be possible regardless if the implementation depends on tunables or wasm features or whatever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants